Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

FIX: severe memory leak in WeakValueMap #336

Merged
merged 1 commit into from
Apr 7, 2015
Merged

FIX: severe memory leak in WeakValueMap #336

merged 1 commit into from
Apr 7, 2015

Conversation

SamSaffron
Copy link
Contributor

entries were not being cleaned up correctly from the backing store. Impact is huge:

ENV['RAILS_ENV'] = 'production'
require 'objspace'
require 'memory_profiler'
require File.expand_path("../../config/environment", __FILE__)

def rss
 `ps -eo pid,rss | grep #{Process.pid} | awk '{print $2}'`.to_i
end

PrettyText.cook("hello world")

# MemoryProfiler has a helper that runs the GC multiple times to make
#  sure all objects that can be freed are freed.
MemoryProfiler::Helpers.full_gc
puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

20.times do

  5000.times { |i|
    PrettyText.cook("hello world")
  }
  MemoryProfiler::Helpers.full_gc
  puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

end

Before

rss: 137660 live objects 306775
rss: 259888 live objects 570055
rss: 301944 live objects 798467
rss: 332612 live objects 1052167
rss: 349328 live objects 1268447
rss: 411184 live objects 1494003
rss: 454588 live objects 1734071
rss: 451648 live objects 1976027
rss: 467364 live objects 2197295
rss: 536948 live objects 2448667
rss: 600696 live objects 2677959
rss: 613720 live objects 2891671
rss: 622716 live objects 3140339
rss: 624032 live objects 3368979
rss: 640780 live objects 3596883
rss: 641928 live objects 3820451
rss: 640112 live objects 4035747
rss: 722712 live objects 4278779
/home/sam/Source/discourse/lib/pretty_text.rb:185:in `block in markdown': Script Timed Out (PrettyText::JavaScriptError)
    from /home/sam/Source/discourse/lib/pretty_text.rb:350:in `block in protect'
    from /home/sam/Source/discourse/lib/pretty_text.rb:348:in `synchronize'
    from /home/sam/Source/discourse/lib/pretty_text.rb:348:in `protect'
    from /home/sam/Source/discourse/lib/pretty_text.rb:161:in `markdown'
    from /home/sam/Source/discourse/lib/pretty_text.rb:218:in `cook'
    from tmp/mem_leak.rb:30:in `block (2 levels) in <main>'
    from tmp/mem_leak.rb:29:in `times'
    from tmp/mem_leak.rb:29:in `block in <main>'
    from tmp/mem_leak.rb:27:in `times'
    from tmp/mem_leak.rb:27:in `<main>'

After

rss: 137556 live objects 306646
rss: 259576 live objects 314866
rss: 261052 live objects 336258
rss: 268052 live objects 333226
rss: 269516 live objects 327710
rss: 270436 live objects 338718
rss: 269828 live objects 329114
rss: 269064 live objects 325514
rss: 271112 live objects 337218
rss: 271224 live objects 327934
rss: 273624 live objects 343234
rss: 271752 live objects 333038
rss: 270212 live objects 329618
rss: 272004 live objects 340978
rss: 270160 live objects 333350
rss: 271084 live objects 319266
rss: 272012 live objects 339874
rss: 271564 live objects 331226
rss: 270544 live objects 322366
rss: 268480 live objects 333990
rss: 271676 live objects 330654

Recommend a new stable release with this asap. Deploying a monkey patch to Discourse

SamSaffron added a commit to discourse/discourse that referenced this pull request Apr 1, 2015
@SamSaffron
PERF: Fix memory leak
d390603 
We used to leak some memory every time you cook a post

see: rubyjs/therubyracer#336
@SamSaffron
FIX: severe memory leak in WeakValueMap
f3b8f84 
entries were not being cleaned up correctly from the backing store
@cowboyd
Copy link
Collaborator

cowboyd commented Apr 1, 2015

If I understand correctly, this is because while the referenced objects are being gc'd, the actual entries containing the week references are not?

@SamSaffron
Copy link
Contributor Author

yes, exactly, blogged a bit about this as well. http://samsaffron.com/archive/2015/03/31/debugging-memory-leaks-in-ruby

novokshonovi
novokshonovi reviewed Apr 3, 2015
View reviewed changes
lib/v8/weak.rb
end

def self.ensure_cleanup(values,key,ref)
proc {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what the reason to use proc {} ? Asking just for educational purpose

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://ruby-doc.org/core-2.2.1/ObjectSpace.html#method-c-define_finalizer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@novokshonovi regarding the reasoning for the class method and rather awkward definition of finalizers, you need to be ultra careful with closures: see http://www.mikeperham.com/2010/02/24/the-trouble-with-ruby-finalizers/

@dedene
Copy link

dedene commented Apr 3, 2015

👍

@cowboyd cowboyd merged commit f3b8f84 into rubyjs:master Apr 7, 2015
@cowboyd
Copy link
Collaborator

cowboyd commented Apr 7, 2015

Released with v0.12.2

@SamSaffron
Copy link
Contributor Author

wow, awesome. time to kill my monkey patch :)

On Tue, Apr 7, 2015 at 1:52 PM, Charles Lowell notifications@github.com
wrote:

Released with v0.12.2
https://rubygems.org/gems/therubyracer/versions/0.12.2

—
Reply to this email directly or view it on GitHub
#336 (comment).

@dsander dsander mentioned this pull request Apr 11, 2015
Closed
ruseel pushed a commit to ruseel/fluent-logger-ruby that referenced this pull request Jul 15, 2015
rm "at_exit"
24ea44f 
at_exit seems to leak memory.

checked with below script
(copied from SamSaffron's
rubyjs/therubyracer#336)

```
require 'objspace'
require 'memory_profiler'
require 'fluent-logger'

def rss
 `ps -eo pid,rss | grep #{Process.pid} | awk '{print $2}'`.to_i
end

MemoryProfiler::Helpers.full_gc
puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"

20.times do
  500.times { |i|
    Fluent::Logger::FluentLogger.new.post("tag", {"a"=>1})
  }

  MemoryProfiler::Helpers.full_gc
  puts "rss: #{rss} live objects #{GC.stat[:heap_live_slots]}"
end
```

before this patch

```
rss: 19528 live objects 38181
rss: 20836 live objects 46999
rss: 21108 live objects 54998
rss: 21948 live objects 62998
rss: 22992 live objects 70998
rss: 24192 live objects 78998
rss: 25248 live objects 86998
rss: 26324 live objects 94998
rss: 27432 live objects 102998
rss: 28556 live objects 110998
...
...

```

after

```
rss: 19840 live objects 38181
rss: 20988 live objects 38998
rss: 21048 live objects 38997
rss: 21056 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
rss: 21116 live objects 38997
...
...
```
@ruseel ruseel mentioned this pull request Jul 15, 2015
Closed
cowboyd added a commit that referenced this pull request Aug 15, 2015
@cowboyd
ensure idenity map entries are cleaned up:
8e39f07 
This adds a regression for:

#336
@cowboyd cowboyd mentioned this pull request Aug 15, 2015
Open
@wasafiri wasafiri mentioned this pull request Jan 14, 2016
Open
@erkde erkde mentioned this pull request Feb 1, 2016
Closed
pnguyen1097 added a commit to pnguyen1097/billion-web that referenced this pull request Apr 25, 2016
@pnguyen1097
Update therubyracer to get rid of memory leak.
c1efc4e 
Reference: rubyjs/therubyracer#336
pnguyen1097 added a commit to pnguyen1097/billion-web that referenced this pull request Jul 18, 2016
@pnguyen1097
Update therubyracer to get rid of memory leak.
c43b62e 
Reference: rubyjs/therubyracer#336
vlexaargas pushed a commit to billioneffect/billion-web that referenced this pull request Aug 19, 2016
@pnguyen1097 @vlexaargas
Update therubyracer to get rid of memory leak.
e2ac876 
Reference: rubyjs/therubyracer#336
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SamSaffron @cowboyd @dedene @ignisf @novokshonovi